Skip to content

Comments

WIP new option list#5510

Merged
willmcgugan merged 21 commits intomainfrom
new-option-list
Feb 12, 2025
Merged

WIP new option list#5510
willmcgugan merged 21 commits intomainfrom
new-option-list

Conversation

@willmcgugan
Copy link
Member

@willmcgugan willmcgugan commented Feb 8, 2025

A rewrite of OptionList internals, mostly preserving the original interface. The original was brittle, small changes would break everything. There was also some misguided attempts at doing things lazily, which resulted in doing work multiple times, particularly when first adding the widget. The original would also not be able to report its size accurately, until two or three frames later. An earlier pass partially fixed that, but it still took a frame before the OL could report its size.

@willmcgugan willmcgugan marked this pull request as draft February 8, 2025 17:52
@TomJGooding
Copy link
Collaborator

TomJGooding commented Feb 11, 2025

I hope you don't mind me sticking my nose in (as usual!), especially since this is still a WiP.

Is it necessary to remove the Separator for this improved version of the OptionList, or is this a design choice? Specifying a separator with None is obviously less intuitive, and it would be a shame to introduce a breaking change if it can be avoided.

@willmcgugan
Copy link
Member Author

I would disagree it is less intuitive. The OptionList intuitively takes a list of options. But a "Separator" is not an option, nor anything that is present in the option list. It is just a way of marking the end of a logical group. I think None does a better job of dividing the options into such groups.

Internally, it does simplify things. There is no need to distinguish between a real option and this special Separator case.

@willmcgugan willmcgugan marked this pull request as ready for review February 12, 2025 11:03
@willmcgugan willmcgugan merged commit d10791d into main Feb 12, 2025
23 checks passed
@willmcgugan willmcgugan deleted the new-option-list branch February 12, 2025 14:13
Comment on lines +3496 to +3497
def test_option_list_wrapping(snap_compare):
"""You should see a 40 cell wide Option list with a single line, ending in an ellipsis."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't spot this before it was merged, from the snapshot it looks like the OptionList height is based on the full height of the prompt rather than the truncated version with nowrap. Is that expected?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants